Skip to content

fix(sqlite-native): restore native startup kv preload#4639

Open
NathanFlurry wants to merge 1 commit into04-12-fix_sqlite-native_delete_metadata_before_chunk_rangefrom
04-12-fix_sqlite-native_restore_native_startup_kv_preload
Open

fix(sqlite-native): restore native startup kv preload#4639
NathanFlurry wants to merge 1 commit into04-12-fix_sqlite-native_delete_metadata_before_chunk_rangefrom
04-12-fix_sqlite-native_restore_native_startup_kv_preload

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Copy Markdown
Member Author

NathanFlurry commented Apr 13, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link
Copy Markdown

claude bot commented Apr 13, 2026

Code Review

The PR restores the native startup KV preload that was previously a no-op (_preloaded_kv was ignored). The overall approach is sound. A few items worth addressing:

Issues from prior review (still present)

startup_preload_put silently ignores new keys without explanation

fn startup_preload_put(entries: &mut StartupPreloadEntries, key: &[u8], value: &[u8]) {
    if let Ok(idx) = startup_preload_search(entries, key) {
        entries[idx].1 = value.to_vec();
    }
    // Err(_) falls through with no insert
}

The no-insert-on-miss behavior is intentional (new pages written at runtime are already durable in KV), but without a comment a reader will assume this is a bug. Add something like: // Do not insert new keys; the preload only tracks entries fetched at startup.

kv_get appends hits after misses, not in input order

result.keys.extend(preloaded_keys);
result.values.extend(preloaded_values);

The merged result places miss keys first (from the network response), then appends hit keys. Confirm that all callers look up values by key rather than by position. If KvGetResult is consumed positionally anywhere, this silently returns wrong values.

No integration-level test for the partial-hit/miss merge in kv_get

The three new unit tests cover helper functions in isolation. Nothing exercises the mixed case: some keys in the preload, some requiring a network fetch. A test with a mock SqliteKv asserting correct key->value pairing after the merge would catch ordering bugs.


New items

Version metadata is encoded, transmitted, then dropped

bridge_actor.rs serialises entry.metadata.version into the JSON payload; database.rs discards it:

.map(|entry| (entry.key.to_vec(), entry.value.to_vec()))

If the VFS preload never needs versions this is fine, but the round-trip cost (base64-encode in Rust, base64-decode in JS, then dropped) is wasted work. Either stop encoding the version field in encode_preloaded_kv, or add a comment explaining why it is intentionally dropped.

requestedGetKeys and requestedPrefixes are decoded but appear unused in the native path

decodePreloadedKv builds the full { entries, requestedGetKeys, requestedPrefixes } object, but openDatabaseFromEnvoy/openRawDatabaseFromEnvoy only accept [Uint8Array, Uint8Array][] (key-value pairs). If these fields are consumed by the WASM path elsewhere, a brief comment would clarify. If they are genuinely unused in the native path, skip decoding them to avoid unnecessary allocations.

Mutex poison silently turns write-through updates into no-ops

fn update_startup_preload(&self, f: impl FnOnce(&mut StartupPreloadEntries)) {
    if let Ok(mut guard) = self.startup_preload.lock() { ... }
    // Err (poisoned) silently falls through
}

If a panic occurs while the lock is held, all subsequent kv_put/kv_delete preload updates are silently skipped while kv_get continues serving the pre-panic snapshot. This matches how last_error is handled elsewhere so it is not a new pattern, but it means a panic can introduce invisible inconsistency. Consider .unwrap_or_else(|e| e.into_inner()) to recover from poison.


Minor nit

wrapper.js decodePreloadedKv uses Uint8Array.from(Buffer.from(value, 'base64')). Buffer.from already returns a Uint8Array subclass; the outer Uint8Array.from iterates every byte to produce a plain copy. Returning the Buffer directly or using new Uint8Array(buf.buffer, buf.byteOffset, buf.byteLength) would be cheaper. Not a correctness issue.

@NathanFlurry NathanFlurry force-pushed the 04-12-fix_sqlite-native_delete_metadata_before_chunk_range branch from 12c0baa to 61b5457 Compare April 13, 2026 05:38
@NathanFlurry NathanFlurry force-pushed the 04-12-fix_sqlite-native_restore_native_startup_kv_preload branch from db38027 to 32f2924 Compare April 13, 2026 05:38
@NathanFlurry NathanFlurry force-pushed the 04-12-fix_sqlite-native_delete_metadata_before_chunk_range branch from 61b5457 to bc7e3d7 Compare April 13, 2026 05:50
@NathanFlurry NathanFlurry force-pushed the 04-12-fix_sqlite-native_restore_native_startup_kv_preload branch 2 times, most recently from 2b4d526 to bd95874 Compare April 13, 2026 07:03
@NathanFlurry NathanFlurry force-pushed the 04-12-fix_sqlite-native_restore_native_startup_kv_preload branch from bd95874 to ebe7c80 Compare April 13, 2026 21:07
@NathanFlurry NathanFlurry marked this pull request as ready for review April 14, 2026 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant